Skip to content

Add string literal completions for package.json imports field - #57718

Merged
Andrew Branch (andrewbranch) merged 27 commits into
microsoft:mainfrom
Andarist:pkg-json-imports-completions
Oct 31, 2024
Merged

Add string literal completions for package.json imports field#57718
Andrew Branch (andrewbranch) merged 27 commits into
microsoft:mainfrom
Andarist:pkg-json-imports-completions

Conversation

@Andarist

@Andarist Mateusz Burzyński (Andarist) commented Mar 11, 2024

Copy link
Copy Markdown
Contributor

closes #52460
closes #57680
closes #57777

Currently, this only has tests based on #55015 but I still have to add more

@typescript-bot TypeScript Bot (typescript-bot) added the For Backlog Bug PRs that fix a backlog bug label Mar 11, 2024
Comment thread src/compiler/utilities.ts
}

/** @internal */
export function getPossibleOriginalInputPathWithoutChangingExt(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is kinda a reverse of getOutputPathWithoutChangingExt:

function getOutputPathWithoutChangingExt(
inputFileName: string,
ignoreCase: boolean,
outputDir: string | undefined,
getCommonSourceDirectory: () => string,
): string {
return outputDir ?
resolvePath(
outputDir,
getRelativePathFromDirectory(getCommonSourceDirectory(), inputFileName, ignoreCase),
) :
inputFileName;
}

Comment thread src/compiler/utilities.ts
Comment thread src/services/stringCompletions.ts Outdated
Comment thread src/services/stringCompletions.ts Outdated
Comment thread src/services/stringCompletions.ts Outdated
Comment thread src/services/stringCompletions.ts
Comment thread src/services/stringCompletions.ts Outdated
Comment thread src/services/stringCompletions.ts
Comment thread src/services/stringCompletions.ts
Comment thread src/services/stringCompletions.ts
Comment thread src/services/stringCompletions.ts Outdated
@Andarist

Copy link
Copy Markdown
Contributor Author

Andrew Branch (@andrewbranch) I opened this as a draft because there are some minor cleanups to be done here. I'd appreciate an early review here though - in case there is something fundamentally wrong with this.

@DanielRosenwasser

Copy link
Copy Markdown
Member

Mateusz Burzyński (@Andarist) did you still want to pursue this? We're looking to get this done in the TS 5.7 timeframe.

@Andarist

Copy link
Copy Markdown
Contributor Author

Daniel Rosenwasser (@DanielRosenwasser) yes, it would be great if Andrew Branch (@andrewbranch) could give this a quick look to check if im not doing anything overly wrong so I dont spend too much time on cleaning up things that will turn out to be wrong in the end ;p

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks on the right track to me, thanks!

Comment thread src/compiler/utilities.ts
Comment thread tests/cases/fourslash/importCompletionsPackageJsonImportsPattern.ts
@Andarist

Copy link
Copy Markdown
Contributor Author

Andrew Branch (@andrewbranch) thanks for the review! I’ll try to clean this up asap

…letions

# Conflicts:
#	src/compiler/utilities.ts
#	src/services/stringCompletions.ts
Comment thread src/harness/vfsUtil.ts
const value = normalizeFileSetEntry(files[key]);
const path = dirname ? vpath.resolve(dirname, key) : key;
vpath.validate(path, vpath.ValidationFlags.Absolute);
vpath.validate(path, vpath.ValidationFlags.Absolute | vpath.ValidationFlags.AllowWildcard);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if this is completely correct. I added it to make pathCompletionsPackageJsonImportsSrcNoDistWildcard6 pass as it's using ? in the component path component.

I have added that test based on the existing pathCompletionsPackageJsonExportsWildcard6. The difference is that the test I have added is a fourslash/server test and it follows a slightly different codepath.

The existing one validates here:

private _resolve(path: string) {
return this._cwd
? vpath.resolve(this._cwd, vpath.validate(path, vpath.ValidationFlags.RelativeOrAbsolute | vpath.ValidationFlags.AllowWildcard))
: vpath.validate(path, vpath.ValidationFlags.Absolute | vpath.ValidationFlags.AllowWildcard);
}

And the ValidationFlags.AllowWildcard was added there here

On the other hand, the fourslash/server validates here, where I'm adding this comment. This codepath was added as part of #20763 - which proceeds the PR that introduced ValidationFlags.AllowWildcard. So perhaps this was just a harmless omission in that newer PR. I don't know why this is a flag in the first place though so 🤷‍♂️

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ron Buckton (@rbuckton) can you advise?

@Andarist

Copy link
Copy Markdown
Contributor Author

Andrew Branch (@andrewbranch) it's ready for re-review :)

I could also use a build of this to test it more easily in a real project, and not only in the test harness ;p cc Jake Bailey (@jakebailey)

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 31, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 31, 2024

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/164040/artifacts?artifactName=tgz&fileId=7D1CD9129128DE0FC3646A76F014BB023412AEA343119330949BF42DF191661C02&fileName=/typescript-5.7.0-insiders.20241031.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.7.0-pr-57718-9".;

@jakebailey Jake Bailey (jakebailey) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay; the FS thing I think is also fine? Everything passes...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Backlog Bug PRs that fix a backlog bug

Projects

None yet

5 participants